[rush] Fix issue where package.json script hangs when accessing STDIN on Windows #5579
Conversation
…accessing `process.stdin`, ultimately caused by Rush spawning with stdin='pipe' yet not properly closing the pipe.
octogonz
requested review from
D4N14L,
apostolisms,
dmichon-msft and
iclanton
as code owners
| } | ||
| }); | ||
|
|
||
| const stdio: child_process.StdioOptions = handleOutput ? ['pipe', 'pipe', 'pipe'] : [0, 1, 2]; |
There was a problem hiding this comment.
We should ABSOLUTELY not be inheriting stdin, that would imply that anything the user types would get unexpectedly forwarded to some arbitrary set of child processes.
There was a problem hiding this comment.
In fact, in general, the else case that inherits stdin is also not a sensible default; the caller should have to expressly specify if they want stdin, otherwise we should default to ignore.
There was a problem hiding this comment.
What you are saying certainly makes sense whenrush build is running shell scripts and piping their output to the summarizer. The problem underlying this PR is that we have many code paths that run scripts, but no rigorous indication of which scenarios should accept STDIN.
Tracing the callers of _executeLifecycleCommandInternal():
| scenario that runs lifecycle script | should accept STDIN? | ILifecycleCommandOptions.handleOutput |
|---|---|---|
| "hook" scripts in rush.json | maybe? originally they were for telemetry only, but what about postRushx? |
!printEventHooksOutputToConsole from experiments.json |
| "global" Rush custom commands | yes | false |
rushx |
yes | false |
ShellOperationRunner for phased builds |
no; @dmichon-msft I think this is what you are focusing on | true |
IPCOperationRunner |
no? this is used to invoke Heft for multi-project watch mode, so I guess interactive commands don't make sense there? | true |
I'm not even sure handleOutput should be what determines this; look at its docs:
export interface ILifecycleCommandOptions {
/**
* If true, suppress the process's output, but if there is a nonzero exit code then print stderr
*/
handleOutput: boolean;Maybe it should be renamed to handleOutputAndIgnoreStdin?
There was a problem hiding this comment.
Your suggestion was this:
- const stdio: child_process.StdioOptions = handleOutput ? [0, 'pipe', 'pipe'] : [0, 1, 2];
+ const stdio: child_process.StdioOptions = handleOutput ? ['ignore', 'pipe', 'pipe'] : [0, 1, 2];It definitely breaks less table rows. The main impact would be Rush global commands, but I suppose that was already broken with pipe?
There was a problem hiding this comment.
I updated the branch with this change.
|
Observations from our discussion:
|
3 participants
Summary
The problem I encountered:
rush install && rush testfrom cmd.exeThe process hangs while building
api-documenter-scenarios, which uses a Heft"run-script-plugin"to invokerunScenarios.tswhich runsnode.exe api-documenter/lib/start.js. The actual hang is somewhere in API Documenter, but if you simply addconsole.log(process.stdin.isTTY)to the top ofapi-documenter/lib/start.jsit will hang on that line forever.Details
Presumably something goes wrong during lazy initialization of the
process.stdinproperty, which probably is a deep Node.js bug on Windows. Windows TTY/streams have unusual semantics, and the Node maintainers don't put as much effort into Windows oddities. I found several GitHub issues about STDIN hangs that were opened and later closed without any code change.A simple way to avoid the hang is to change this code:
rushstack/build-tests/api-documenter-scenarios/src/runScenarios.ts
Lines 46 to 59 in be05af7
...to use
stdio: ['ignore', 'inherit', 'inherit'].But I did not make that fix, because it seems fundamentally unreasonable: What's wrong with inheriting STDIN? And why does it only repro when invoked by Rush?
After a bunch of debugging, I realized Rush is doing something wrong:
When spawning a child process, in the case where it binds STDOUT/STDERR to
pipeto redirect the output to the stream collator, Rush also seems to bind STDIN topipe. BUT Rush never actually attaches any handlers to the STDIN stream, nor does it close it properly. This doesn't justify the Node.js hang when accessingstdin.isTTY, but it does raise the possibility of a hang if the child process tries to read STDIN and expects the stream to get closed later, since it's not a TTY.Looking around in
Utilities.ts, there seem to be other copy+pastes of this same mistake, however I didn't have time to analyze them deeply. If someone else wants to suggest other fixes, that would be very welcome.How it was tested
Hang repros without the fix. Build does not hang with this fix.